Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gpio command controller #1251

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Gpio command controller #1251

wants to merge 48 commits into from
+1,191 −0

Conversation

Wiktor-99
Copy link

@Wiktor-99 Wiktor-99 commented Aug 17, 2024 •
edited
Loading

As I discussed with @christophfroehlich I've opened new pr with gpio controller.

This PR is a follow-up to the thread and implements a controller to send commands to GPIO interfaces, allowing specific command interfaces for each GPIO.

I have made the following changes compared to the original PR:

  • I utilized the 'DynamicJointState' message for both the command and state interfaces (this allows sending commands with specific GPIO values without having to worry about the order of ports in the command message).
  • I added a parameters file, and the new input parameters file will look something like this:
gpios:
  - gpio1
  - gpio2
command_interfaces:
  - gpio1:
    - ports:
      - dig1
      - dig2
  - gpio2:
    - ports:
      - ana1
  • I've done significant refactoring and have added many new UTs.

mcbed and others added 30 commits July 26, 2024 09:25
@mcbed @christophfroehlich
added gpio_command_controller
5fecf27
@mcbed @christophfroehlich
added doc to gpio controllers
6ffda9d
@mcbed @christophfroehlich
changed parameter interface_names to command_interfaces
f88fc7d
@mcbed @christophfroehlich
added test
56fc1e5
@mcbed @christophfroehlich
code formating
9e73ab7
@christophfroehlich
added gpio state publisher
1cbd3a6
@christophfroehlich
fixed broken node_state
7228e06
@christophfroehlich
added DynamicJointState message for Gpio state
880aeea
@christophfroehlich
fixed tests and added gpio state tests
94befb0
@christophfroehlich
fixed format
882c936
@Wiktor-99
Apply pre-commit fix
efce87a
@Wiktor-99
Fix compilation on ros rolling
b53bff6
@Wiktor-99
Add parameters file, use generated gpio_command_controller_parameters…
67b6069 
… params and align UTs
@Wiktor-99
Remove unnecessary members: interface_names_, gpios_names_ and use pa…
30d2604 
…rams struct instead.

Use default member initializer.
@Wiktor-99
Refactor of storing interface types
4c824e8
@Wiktor-99
Remove unnecessary includes
dd86d9e
@Wiktor-99
Refacotr on_activate method. Add validate_configured_interfaces and i…
4ac938d 
…nitialize_gpio_state_msg methods
@Wiktor-99
Refactor update method, split update of state if and command if
aa59297
@Wiktor-99
Move test fixture form hpp to cpp
6276617
@Wiktor-99
Update docks and package.xml
66bd971
@Wiktor-99
Add state validation
7b70159
@Wiktor-99
Store command and state ifs in the map
58c1afc
@Wiktor-99
Use interface maps for command and state update. Use DynamicJointStat…
e5c65db 
…e as cmd type
@Wiktor-99
Use ordered state if vector instead of map
0dc5c87
@Wiktor-99
Restore state if validation
765a3cb
@Wiktor-99
Align logging
a1c8fa3
@Wiktor-99
Update code layout
de8acba
@Wiktor-99
Simplifie test fixture
aebbf0b
@Wiktor-99
Update tests. Stop using pointer, set members as public
0deb417
@Wiktor-99
Add set of test for init
3e7a12a
olivier-stasse
olivier-stasse reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. The unit tests looks pretty extensive.
It would be nice if a maintainer could allow the CI to run on your PR.

gpio_controllers/package.xml Outdated Show resolved Hide resolved
@Wiktor-99
Copy link
Author

There was some issue with includes I've fixed it so jobs can be run more time.

@Wiktor-99
Copy link
Author

I've fixed docks and pre-commit jobs other two jobs failed due to pid controller and ackermann steering controller.
I believe that now CI should be green.

@Wiktor-99
Copy link
Author

@christophfroehlich could you rerun the CI?

@Wiktor-99
Copy link
Author

@christophfroehlich could you rerun the CI?

It appears that the CI is unstable and is failing on the diff drive and pid controller.

@christophfroehlich
Copy link
Contributor

@christophfroehlich could you rerun the CI?

It appears that the CI is unstable and is failing on the diff drive and pid controller.

Binary builds due to a recent API breaking change. Semi-binary seems to be happy.

@Wiktor-99
Copy link
Author

@christophfroehlich could you rerun the CI?

It appears that the CI is unstable and is failing on the diff drive and pid controller.

Binary builds due to a recent API breaking change. Semi-binary seems to be happy.

Okay, so what steps should be taken to merge the pr?

@christophfroehlich
Copy link
Contributor

Someone has to find time to test and review it..

@codecov Codecov
Copy link

codecov bot commented Sep 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.69369% with 21 lines in your changes missing coverage. Please review.

Project coverage is 80.81%. Comparing base (50036e1) to head (a2b1f4f).

Files with missing lines Patch % Lines
gpio_controllers/src/gpio_command_controller.cpp 85.12% 16 Missing and 2 partials ⚠️
..._controllers/test/test_gpio_command_controller.cpp 98.54% 0 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1251      +/-   ##
==========================================
+ Coverage   80.36%   80.81%   +0.45%     
==========================================
  Files         105      108       +3     
  Lines        9390     9723     +333     
  Branches      828      843      +15     
==========================================
+ Hits         7546     7858     +312     
- Misses       1570     1586      +16     
- Partials      274      279       +5
Flag Coverage Δ
unittests 80.81% <93.69%> (+0.45%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rollers/test/test_load_gpio_command_controller.cpp 100.00% <100.00%> (ø)
..._controllers/test/test_gpio_command_controller.cpp 98.54% <98.54%> (ø)
gpio_controllers/src/gpio_command_controller.cpp 85.12% <85.12%> (ø)

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivier-stasse olivier-stasse olivier-stasse left review comments

@bmagyar bmagyar Awaiting requested review from bmagyar

@christophfroehlich christophfroehlich Awaiting requested review from christophfroehlich

@destogl destogl Awaiting requested review from destogl

@mcbed mcbed Awaiting requested review from mcbed

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

@progtologist progtologist Awaiting requested review from progtologist

@saikishor saikishor Awaiting requested review from saikishor

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Wiktor-99 @christophfroehlich @olivier-stasse @mcbed @bmagyar